Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sidebarnav #750

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Sidebarnav #750

wants to merge 7 commits into from

Conversation

philippark
Copy link
Collaborator

Scrolling sidebar files with keyboard

Copy link
Contributor

@curran curran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a review of the details. Overall nice work!

When I test it, here's what I see:

image

image

We seem to have lost the 100% width styling on the sidebar entries.

image

Also, there is a strange purple highlight over what looks like a parent div when I click. That does not look that good. Perhaps it could be made to match the border radius of the entries.

Also, how do I navigate through the files with the keyboard? I found that if I click on one, then use the arrow keys, it does not work as expected. I would expect the arrow keys to select between files, but it does not. I'm curious what interactions you are using to test this. Thanks!

package.json Show resolved Hide resolved
}

.list li:nth-child(2n) {
background: var(--spectrum-alias-highlight-hover);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the --spectrum-alias-highlight-hover definition coming from? Did you verify that this resolves correctly? We may need a CSS import to make this work.

@@ -94,6 +100,48 @@ export const VZSidebar = ({
handleDrop,
} = useDragAndDrop();

function List(props) {
let state = useListState(props);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The style of this codebase prefers const over let. Please prefer const where possible. Thanks!

src/client/VZSidebar/index.tsx Show resolved Hide resolved
src/client/VZSidebar/index.tsx Outdated Show resolved Hide resolved
src/client/VZSidebar/index.tsx Show resolved Hide resolved
src/client/VZSidebar/index.tsx Show resolved Hide resolved
src/client/VZSidebar/index.tsx Show resolved Hide resolved
src/client/VZSidebar/index.tsx Show resolved Hide resolved
aria-label="Example List"
selectionMode="multiple"
selectionBehavior="replace"
onAction={(key) => handleFileDoubleClick(key)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one can be simplified.

Suggested change
onAction={(key) => handleFileDoubleClick(key)}
onAction={handleFileDoubleClick}

@curran curran self-assigned this Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants